Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement HTTP server for task processing #58

Merged
merged 1 commit into from
Jun 4, 2024
Merged

implement HTTP server for task processing #58

merged 1 commit into from
Jun 4, 2024

Conversation

dmitsh
Copy link
Collaborator

@dmitsh dmitsh commented Jun 2, 2024

This PR adds server functionality to knavigator.

To start the server, run "knavigator -port <port#>", for example

knavigator -port 12345

Then you can start executing tasks by issuing HTTP requests, for example

curl -v -H "Content-Type: application/x-yaml" --data-binary @test-job.yml http://localhost:12345/workflow

or, alternatively, use the client:

klient -address "http://localhost:12345" -workflow resources/tests/k8s/test-job.yml

@dmitsh dmitsh requested a review from lalitadithya June 2, 2024 02:56
@dmitsh dmitsh force-pushed the ds-srv branch 22 times, most recently from f4491c2 to 6703522 Compare June 2, 2024 16:34
}

func execReset(urlPath string) error {
resp, err := http.Get(urlPath) // #nosec G107
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a code comment on why we need #nosec G107

Copy link
Collaborator

@lalitadithya lalitadithya Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your changes, thanks for adding them. I meant we should also add why are we ignoring G107. Something along the lines of "Since the URL is provided by the same user who wants to use the system and not by a third party, a malicious redirect will not be possible". ref https://securego.io/docs/rules/g107.html

We should do the same in other places as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't it obvious? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but actually, since we pass address as a command line parameter, it could be a security issue. But not for the fake cluster.

cmd/knavigator/main.go Outdated Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
log *logr.Logger
}

type TaskHandler struct {
Copy link
Collaborator

@lalitadithya lalitadithya Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the API is simple, instead of using structs, can we use functions as HTTP handlers? Please see https://www.alexedwards.net/blog/an-introduction-to-handlers-and-servemuxes-in-go (section "Functions as handlers")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can use functions because I need to call eng from the handler.

pkg/server/server.go Show resolved Hide resolved
cmd/klient/main.go Outdated Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
g.Add(
func() error {
srv.log.Info("Starting server", "address", srv.s.Addr)
return srv.s.ListenAndServe()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need some form of authn and authz? I think a user who is able to access the server will be able to privilege escalate since the user and the server use two different RBAC means. What are your thoughts on this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to complicate things. This is a local kind cluster, the nodes are virtual. I think authn/authz will be an overkill.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, fair point. Then we should add documentation describing the risks for users who want to have a long running cluster, maybe for simulation testing/QA.

I was also wondering; can we leverage the JWT tokens generated for Kubernetes service accounts? Use the tokens for a service account (https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#manually-create-an-api-token-for-a-serviceaccount) for making calls to the Kubernetes API and let the Kubernetes API server handle auth. User will provide the token as a part of the Authorization header.

@dmitsh dmitsh force-pushed the ds-srv branch 5 times, most recently from c135105 to 27bb687 Compare June 3, 2024 23:20
@dmitsh dmitsh force-pushed the ds-srv branch 2 times, most recently from e5fee69 to 70772aa Compare June 3, 2024 23:27
@dmitsh dmitsh requested a review from lalitadithya June 3, 2024 23:31
@dmitsh dmitsh force-pushed the ds-srv branch 2 times, most recently from c21015c to aea7112 Compare June 4, 2024 05:01
@dmitsh dmitsh merged commit b7370ee into main Jun 4, 2024
4 checks passed
@dmitsh dmitsh deleted the ds-srv branch June 4, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants